Validate how a global is called, not just that it exists (#104) - #451
Merged
Conversation
#450 taught both validators the eight global *names*, which stopped `{{ now() }}` -- a pipeline that runs correctly -- from being reported as an undefined variable. It stopped at the name, and so accepted every other way of writing one: {{ nowx() }} rejected (not a global) {{ now.foo }} accepted -> fails at run time {{ file_exists.bad }} accepted -> fails at run time {{ now(1, 2, 3) }} accepted -> fails at run time {{ file_exists() }} accepted -> fails at run time {{ now }} accepted -> *runs* The last one is the one worth the PR. Nothing fails: the repr of a live function object is written into the artifact where a timestamp was meant to go -- <function TemplateManager._setup_custom_filters.<locals>.now at 0x1084...> It is not the #447 escape returning: `now.__globals__` and `now.__class__` are both refused by the sandboxed environment, and tests now pin that. What leaks is the repr, not the object graph. The text cannot tell these apart -- `now` is spelled identically in `{{ now() }}`, `{{ now.foo }}` and `{{ now }}` -- so the check reads the parsed Jinja AST and asks whether each Name node *is the callee of a Call*. Identity, not spelling: `{{ now() }}{{ now.foo }}` has two nodes with the same name, one valid. `core/template_globals.py` declares the language: name, argument contract, summary, following `core/actions.py`. That reverses #450, which derived the names from whatever `TemplateManager` registered. Derivation could not drift but could not express an arity either, and it made the public language a shadow of a private implementation detail. Two tests keep what derivation gave for free: the declared names must equal what the runtime registers, and each declared arity must match the callable's real signature. `{% for now in items %}{{ now }}{% endfor %}` binds the name rather than using ours. The binding is a `store` but the use inside the body is an ordinary `load`, indistinguishable without tracking scope, so a template that binds the name anywhere is left alone. Deliberately conservative: a false rejection here is the exact failure the last three changes to this validator existed to remove. Errors carry stable codes -- `global_not_called`, `global_wrong_arity` -- rather than matching on message text. docs/template_globals.md is generated from the same specs the validator checks against, following scripts/generate_action_docs.py, with the same --check test so it cannot drift. Measured: blocking suite 616 -> 670 passed, 0 failed. Catalogue unchanged at 20/117 -- the new strictness rejects no example that validated before. Five mutations, all killed: - unwire the check from the validator -> 4 tests - disable the arity check -> 8 tests - match the callee by name, not identity -> 1 test - declare file_exists as taking 0 args -> 4 tests, incl. the docs check - drop the shadowed-name exclusion -> 3 tests The wiring tests exist because without them, deleting the validator's call site leaves every other test in the file passing -- the checks would be testing the function rather than the validation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses the finding on #450: the validators recognise global names but not how they are used.
Confirmed, plus three more
{{ nowx() }}{{ now.foo }}{{ file_exists.bad }}{{ now(1, 2, 3) }}{{ file_exists() }}{{ active_loops.x.y }}{{ now }}The last row is the one worth the PR. Nothing fails — the repr of a live function object is written into the artifact where a timestamp was meant to go:
It is not the #447 escape returning. I checked before assuming:
What leaks is the repr, not the object graph. Tests pin that, at render time rather than validation time, so it holds for templates that never met the validator.
Why the AST
The text cannot tell these apart —
nowis spelled identically in{{ now() }},{{ now.foo }}and{{ now }}. The check reads the parsed Jinja AST and asks whether eachNamenode is the callee of aCall. Identity, not spelling:{{ now() }}{{ now.foo }}has two nodes with the same name and only one is valid. Mutating that to a name lookup is mutation 3 below.Ownership inverted, as recommended
core/template_globals.pydeclares the language — name, argument contract, summary — following thecore/actions.pyconvention. #450 derived the names from whateverTemplateManagerregistered; that could not drift but could not express an arity either, and it made the public language a shadow of a private implementation detail.Two tests keep what derivation gave for free: declared names must equal what the runtime registers, and each declared arity must match the callable's real signature (
inspect.signature), so the contract cannot claim something the function does not do.Conservative about shadowing
{% for now in items %}{{ now }}{% endfor %}binds the name rather than using ours. The binding is astore, but the use in the body is an ordinaryloadand is indistinguishable without tracking scope — so a template that binds the name anywhere is left alone. A false rejection here is precisely the failure the last three changes to this validator existed to remove. Covered forfor,setandmacro.Also
global_not_called,global_wrong_arity) rather than message matching.docs/template_globals.md, generated from the same specs the validator checks against, with the--checkdrift test thatdocs/actions.mduses.Measured
Mutations — five, all killed
file_existsas taking 0 argsThe four wiring tests exist because without them, deleting the validator's call site leaves every other test in the file passing — they would be testing the function rather than the validation.
Next
Typed
RuntimeContext. Groundwork from #450: seven sites build anexecutiondict in four timestamp formats, andexecution.timestampis rebuilt per task, so two steps in one run disagree (20:01:55.182681vs20:01:55.184368).now's doc entry already points atexecution.timestampfor the deterministic case.🤖 Generated with Claude Code